Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PKCE auth flow, code cleanup #10

Merged
merged 5 commits into from
Mar 31, 2021
Merged

Conversation

javfg
Copy link
Contributor

@javfg javfg commented Mar 31, 2021

This PR addresses #9. Both using PKCE flow in compatible instances of Indico, and sending the token in the Authorization header.

Other improvements/fixes:

  • Re-indent to 2 spaces
  • Added headers on files missing it, updated
  • Split services/controllers into their own files
  • Capture more errors in the user path
  • Fixed information display in registrant display (issue described in Use ticket_price instead of amount_paid in registrant.html #7)
  • Make use of OAuth metadata endpoint when available, allowing for smaller QR codes from now on
  • Dropped hellojs, jsOAuth and moment

closes #7, closes #9

www/js/controllers/navigationController.js Outdated Show resolved Hide resolved
www/js/controllers/registrantController.js Outdated Show resolved Hide resolved
www/js/util.js Outdated Show resolved Hide resolved
www/js/controllers/registrantsController.js Outdated Show resolved Hide resolved
www/js/services/indicoApiService.js Outdated Show resolved Hide resolved
www/js/util.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
www/js/services/indicoApiService.js Outdated Show resolved Hide resolved
www/js/services/indicoApiService.js Outdated Show resolved Hide resolved
www/js/controllers/eventsController.js Outdated Show resolved Hide resolved
www/js/util.js Show resolved Hide resolved
www/partials/registrant.html Outdated Show resolved Hide resolved
www/partials/registrant.html Outdated Show resolved Hide resolved
ThiefMaster pushed a commit to indico/indico that referenced this pull request Mar 31, 2021
@ThiefMaster ThiefMaster merged commit 5bd9431 into indico:master Mar 31, 2021
DirkHoffmann added a commit to DirkHoffmann/indico that referenced this pull request Apr 14, 2021
* Let cloners know about multiple clones

And bypass VC cloner availability check after the 1st clone (useful for
e.g. Zoom)

* Patch plugin version suffix in setup.cfg

* Stop setting unnecessary click option

The warning is only triggered in Python 2 with unicode_literals

* Update dev docs for Python 3

* Show timetable filter in meeting-like conferences

* Rename copyright.yml to headers.yml

The old filename broke GitHub's license detection

* Move package metadata from setup.py to setup.cfg

Except for some dynamic parts...

* Pin versions of dev dependencies

* Expose dev requirements via [dev] extra

* Sort requirements.dev.txt entries

* Move test dependencies to requirements.dev.txt

And get rid of unused pojson dependency

* Pin minimum versions of pytz and certifi

* We no longer have *.tpl files

* Do not cache JS i18n data for invalid locales

Otherwise each request for an invalid locale will create a cache file.
Security scanners and similar tools may request a lot of invalid locales,
and thus create a large amount of small but useless files in the cache
folder (which are eventually cleaned up but not creating them to begin
with is clearly better).

* Fix downloading abstract/paper file packages

The temporary file was being deleted too early when using xsendfile

* Fix review_editable_revision outside request ctx

* Update Chinese translation 🇨🇳 🐉

* Add missing PR reference to changelog

* Update French translation 🇫🇷 🥖

* Fix changelog wording

* Release 2.3.3

* Bump version to 2.3.4-dev

* Call metadata_postprocess for category event export

When exporting all events from a category extending the metadata may
also be useful, e.g. to add video conference join links.

* Update url_for to default _external to False

Flask's url_for _external defaults to True if there is no request
context.

* Make legacy event redirects routing-agnostic

When changing the routing rules to require numeric event ids, we can no
longer get leading-zero or non-numeric event ids from view_args

* Always consider leading zeroes in event ids legacy

Otherwise we can access a non-legacy event both with and without a
leading zero, which is messy because there should be exactly one
canonical URL

* Replace confId with event_id everywhere

Also make it numeric in the Flask routing rules

* Update Jinja2

The previous version will soon receive a CVE which is not relevant for
us, but let's make tools that only look at the version (e.g. GitHub's
security notifications) happy.

* Fix conference VC page if the VC plugin is missing

* Fix deleting events with a VC room without the plugin

* Update bleach to 3.3.0

The security issue does not affect our usage, but let's avoid warnings
that may scare people...

closes indico#4770

* Remove non-redis cache backends

* Scope cache based on the indico version

Like this we never risk errors due to cached data being from a previous
version.

* Add scoped caching layer on top of flask-caching

Also improve the flask-caching behavior to not fail noisily in case
redis is unreachable and support returning default values in case of
cache misses.

* Replace GenericCache with new scoped_cache

* Remove GenericCache

* Be more forgiving with invalid locales

And warn if the configured default locale is invalid

* Move indico.util.struct.* to indico.util.*

* Add ID-1 page size for badge printing

* Dashboard ical: Only query data that's needed

* Fix eslint issues in Export.js

* Hide semi-broken persistent ical links for contribs

- They pointed to the event ical (without any contribution details)
- When enabling (persistent) API keys from that widget, the URLs
  generated pointed to a legacy contribution-only ical api which 503s
  since v1.9.x
- Persistent calendar links for a single contribution are not
  particularly useful

* Fix event creation

* Fix error in survey results

- can't multiply strings with floats
- dict_keys object can't be jsonified

* Replace OrderedDicts with regular dicts (indico#4778)

Regular dicts are sorted as of Python 3.7, so there is no need to use
OrderedDict anymore.

* Fix abstract submission

`_.pluck()` was behaving very weirdly, and was supposedly removed in our
lodash version anyway...

* eslint person_link_widget.js

* Allow timedelta for cache expiry

* Use sane defaults in create_application fixture

* Allow rejecting registrations with a reason (indico#4769)

* Update *.pot files

* cache: Fix timedelta conversion logic

* cache: Fail noisily while in debug mode

* Add temporary coverage file to gitignore

* Use redis cache backend when running tests

Using different backends in test and dev/prod is likely to result in
errors that happen during regular usage but not in tests - and the fact
that some tests actually contained bugs but passed nonetheless, proves
this.

* Support attaching iCal files to reminder/reg mails (indico#4780)

* Fix dump_url_map

That script initializes the app with testing=True, but we aren't inside
pytest so we have no redis to connect to.

* Fix dict ducktyping

In Python 2 we used to check for iteritems+get which was safe, but
items+get exists for quite a few motels, like the Survey one, and thus
broke the ducktyping there and thus resulted in weird errors.

* Avoid querying active registrations if not needed

* Use 3.9 for RTD build

* Remove unused schema instance

* Remove weird session fallback in /api/user/

It was never used, and the idea of using the session user if there is
any error with the oauth token (missing, invalid, whatever) is extremely
obscure and prone to errors.

* Add unit test for the complete oauth flow

* Use Authlib for OAuth provider

The implicit flow has been replaced with the more modern and in case of
mobile apps more secure code+pkce flow.

* Add token introspection endpoint

* Disallow 'plain' PKCE code challenge

It's less secure and we do not really care about clients that cannot
create a SHA256 hash...

* Add well-known oauth2 metadata endpoint

* Allow CORS on token endpoint

The PKCE flow can be used in browsers, so they should be able to
exchange the code for a token via CORS

* Improve error handling during authorization

* Return more standard oauth errors

* Remove obsolete oauth errors endpoint

* Move oauth provider logic to core & split modules

* Fix test_client with no webpack manifest

If a test results in a templte being rendered this usually looks up some
webpack bundles which we don't care about during tests and don't have
during CI.

* Add revocation endpoint

* Apply app scope restrictions to existing tokens

* Rename default_scopes to allowed_scopes

The old name was just poor naming on the flask-oauthlib side, and never
had anything to do with default scopes.

* Support different tokens for different scopes

Also refactor how user app authorizations are stored; instead of
checking for a token, we now have a separate model holind the
authorization and granted scopes, so if an app revokes its own token,
the scopes will remain authorized in any future authorization flow the
user does.

* Remove unused property

* Remove TODO comment about revoking tokens

Deleting is fine, our tokens are unique so there is basically no risk of
regenerating the same token that has been revoked before.

* Fix error when jsonifying certain oauth errors

* Add tests for insecure oauth logic

* Stop expunging user from db session in tests

Not sure why it now works without that ugly hack...

* Add some app/user link assertions to the tests

* Display metadata URL and add clipboard copy links

* Remove unused authlib method

upstream removed it as well

* Make PKCE configurable

Apps that are not targeting SPA/native apps do not need to allow flows
without a client secret.

* Test token reuse

* Store OAuth tokens as SHA-256 hashes

While not passwords, they are sensitive and there is no need to store
plaintext versions of them.

* Support implicit grant for the checkin app

This also adds support for passing the token insecurely in the query
string, but also ONLY for that app.

Like this we are not under any pressure to update the app, and we can
simply do it at some point after a proper Indico v3 release.

* Add changelog entry for oauth

* Fix merging users with oauth app auths

* Use cascading FKs in oauth tables

* Remove flower integration

* ci: pin ubuntu version

ubuntu-latest is now ubuntu 20 which doesn't work well with python 2

* ci: cache wheels + generate envs in setup

This avoids weird random breakage with cached virtualenvs and is similar
to what we already do for plugins

* Remove deprecated registration form tab related css

* Reduce label scope in the core css

* Add primary_email_changed signal (indico#4802)

Co-authored-by: Adrian Moennich <[email protected]>

* Improve testing behavior

- database: prevent rollbacks
- test client: run each request in a separate app context
- add `make_test_client` fixture to create new clients

* Unify current-request-user logic

* Better handle auth fails during exception handling

In that case the original exception is likely more relevant so we avoid
re-raising the authentication error but rather silently discard the user
that failed to authenticate.

* Remove legacy url signing as much as possible

We keep accepting the old links for the dashboard ical feed but no
longer generate such URLs.

* Simplify data in user_token

We do not need to encode the data in the token, a simple signature is
actually enough since the user id is prepended to the signature part of
the token

Also use sha256 instead of sha1 for the HMAC signature. While SHA1 is
still fine for HMAC purposes, using a stronger algorithm doesn't have
any disadvantages and the tokens are still reasonably short.

* Remove leading whitespace in registrants export

Happened if someone had no title

* Handle oauth tokens in query strings

To be removed when cc2cab2 gets reverted (or earlier if we can ditch
tokens-in-querystring before the implicit flow itself)

* Use new user logic in oauth registrants API

The CSRF checks no longer need to be disabled manually because any
oauth-authenticated request is no longer subject to CSRF checks.

* Do not register user picture url without user id

We always put a user id in that url, and accessing it without one
actually failed with a KeyError

* Use new user logic in user info api

* Remove override_request_user

Looks like we don't need it in the end...

* Preserve oauth errors if a bearer token is used

The oauth RFCs actually specify statuscodes, so replacing everything
with 400 is ugly

* JSON requests are not likely seen by users

* Make session user APIs clearer

* Add changelog for session.user logic change

* Correct some missing whitespace in templates

* Fix URL signing when not logged in

* Improve error handling in url signing

* Use token-based urls for persistent ical links (indico#4801)

Co-authored-by: Adrian Moennich <[email protected]>

* Fix registration form management

* Exclude some user agents from sso redirection

* Add bulk export for editing data to JSON

* UI: add horizontal scroll to body

Provide scrollbar if content overflows.

* Allow Editing team to see editables of unpublished contribs

* Update indico-sui-theme

* Remove obsolete contextlib2 dependency

ExitStack is part of contextlib in Python 3

* Use pip-tools to manage requirements.txt

* Update Python dependencies

* Use new flask-caching cache factory

* Update to Flask-Multipass without open redirects

* Fix open redirects in signup/logout

* Enforce the BASE_URL to match the actualr equest

- Always set `SERVER_NAME` and `APPLICATION_ROOT`
- Improve the handling of requests where this does not match (fail with
  a meaningful error)

* Update French translation 🇫🇷 🥖

* Redirect to canonical URL in the web server

* Release 2.3.4

* Bump version to 2.3.5-dev

* Add 2.3.5 changelog stub

* Fix react warning about duplicate key

* Add better password security checks

* Check password security during login

* Add autocomplete="{new,current}-password" metadata

* Add Flask-Limiter for rate limiting

* Add rate limiting for failed login attempts

* Fix incorrect cache/limiter initialization

This broke building assets

* Only disable jest tests instead of all JS tests

We try building the assets which catches errors such as the url map
extraction script being broken

* Fix error on contribution author page

* Fix static program page having header+footer

* Don't show deleted long-lasting events in calendar

* Allow filtering participant roles by unregistered

* Fix custom role members not showing as registered

(unless they had another event role)

* Add bulk export for paper peer reviewing data to JSON

* Fix oversized badges on buttons

fixes one of the issues in indico#4821

* Fix i-button-label losing boldness on hover

fixes one of the issues in indico#4821

* Improve iCal metadata (indico#4820)

* Do not emit empty location property (indico#4785)
* Include contact data in iCalendar (indico#4786)
* Include event logo url in public events (indico#4787)

* Fix icalendar property name

* Update robots.txt

* Remove legacy xmlGateway endpoint

* Fix nonsense in the docs

* Allow unstaging users by clicking them again

* Allow unstaging users from staged users list

* Flash the staged user count label when it appears

Like this people are more likely to notice it

* Editing: Only show menu links the user can access

* Fix weeks view theme

* Check for duplicate users during registration import

* Fix "inheriting acls" info

- show the message about inherited access again
- properly show each acl type when viewing the list
- remove obsolete code

* Remove weird `is_*` attrs on principal models

Using the enum is much cleaner, and they were only used in few places
(which are now using the principal_type).

* Allow groups/roles as authorized abstract submitters

* Use StrictUndefined for top-level objects

* Be explicit about undefined callers

* Be a bit more strict with undefined attrs/items

* Add tests for our undefined logic

* Celery: execute tasks immediately while testing

* Fix error when notifying new paper reviewing roles

* Use new sentry-python SDK instead of raven

* Fix user id not being logged with empty session

* Fix typo in changelog

* Disallow .bin for "all formats" http api endpoints

It's specifically meant for the file apis, and for anything else it
attempts to send the dict with the original data, which fails with an
ugly error.

* Fix error with flask-limiter and partials

We do not use request-level rate limiting at the moment, so just
disabling the before-request check avoids this bug which broke endpoints
using `RHSimple.wrap_function`.

Related upstream issue: alisaifee/flask-limiter#124

* Update lxml and cryptography

Both updates contain security improvements

* Fix incorrect i18n formatting

* Fix Undefined error in registrant list

* Redirect meeting elements to slug-based URL

* Fix error when marking registration as paid

* Update event QR code for latest checkin app (indico#4844)

Related checkin app PR: indico/indico-checkin#10

* Remove legacy oauth features for checkin app (indico#4846)

* Revert "Handle oauth tokens in query strings"

This reverts commit e1e8dbb.
Also fixes the unit test since the /api/user/ endpoint returns null if
not authenticated instead of failing with 40x

* Revert "Support implicit grant for the checkin app"

This reverts commit cc2cab2.

* Fix another UndefinedError

* Fix subcontribution count check

* Fix another UndefinedError

* Add download option to ical export popups (indico#4850)

* Fix UndefinedError

* verbose_iterator: Support printing total duration

* verbose_iterator: Fix error in case of no items

* Update urllib3 (dependabot alert)

* Remove invalid property

* Remove horizontal scrollbar in Firefox

* Remove unused get_nested_attached_items util

indico/indico-plugins#109 removed the need for it, and in any case it
would be something for the plugin if ever needed again.

* Fix timetable details with explicit session access

When a user has no access to the event but to a session, they should be
able to view the timetable entry details for the contributions within
that session.

* Fix contribution access with explicit-only access

ie a user who cannot access the event itself but is on the ACL for
a contribution

* Hide registration menu item if user has no access

When an event is protected and the user has only access to a specific
session/contribution inside, it makes no sense to show the registration
menu unless the registrations are actually available to people with no
event access.

* Do not show empty conference menu

We still have the blank area, but avoid showing just the border of the
menu list.

* Fix typo

* Create SECURITY.md

* Avoid inconsistent state when file deletion fails

This can happen if there's a race condition between something claiming
the file (and storing a reference to it in FK) + marking it as claimed
and the file being deleted.

Now we still fail in such a case, but no longer keep a file in the
database that's gone from storage.

* Fix reinitializing FileManager during "make changes"

The tags are not guaranteed to be sorted, so sometimes the re-render
caused by `setLoading(true)` resulted in the final-form's initialValues
changing and thus reinitializing the form, triggering the file manager's
logic to reset it as well and delete any pending files.

Since this happened in parallel with the submission of the reviewing
action, it at least resulted in the deleting request failing, but often
also in a race between the deletion request and it checking whether the
file has been claimed and the review claiming it, which resulted in the
file being deleted from storage but not the database (fixed in a
separate commit).

* Add an option to keep Editing team anonymous

* Add support for Polish translation 🇵🇱

* Add Polish translation 🇵🇱

* Update python deps; pin some trickier ones

* Update to SQLAlchemy 1.4

* pytest: Only ignore a specific sqlalchemy warning

* Fix some deprecation warnings

* Cover `unknown` and mutability in webargs tests

* Update to webargs 8

* Fix error when querying room statistics

* Remove likely unnecessary joinedload

This broke with SA 1.4 (sqlalchemy/sqlalchemy#6253) but doesn't
seem to be necessary anyway.

Also added some tests. They are kind of dumb (not testing the actual
filtering logic) but they do cover the part that was broken.

* Fix various Room Booking issues (indico#4861)

* Make sure past bookings can't be cancelled
* Show correct numbers when cancelling bookings
* Fix timeline popup skidding
* Fix fetchActiveBookings params

* Ellipsize plugin version

* Update JS deps within semver

Pinned react-leaflet-draw to a specific version since newer versions
within the 0.19.x line require react-leaflet v3...

* Update package-lock.json

* Update JS deps with semver-major changes

* Update package-lock.json

* Update package-lock.json to v2 (npm 7)

* Update package-lock.json (npm upgrade)

* Replace obsolete shortid package with nanoid

* Fix eslint config

The latest prettier plugin is no longer split into separate subconfigs

* Enable legacy-peer-deps in npmrc

enzyme and useAxios expect older react versions as peer deps
and we don't care about this...

* Add (n)pgettext support

* Apply i18n to category event list date formats

* Update *.pot files

* Regenerate package-lock.json & fix ssh urls

* `npm update` once again

* Run latest pyupgrade

* Use correct secure_filename fallback values

* Be more verbose if plugin assets have not been built

Co-authored-by: Pedro Ferreira <[email protected]>
Co-authored-by: Adrian Moennich <[email protected]>
Co-authored-by: Indico Team <[email protected]>
Co-authored-by: Pedro Lourenço <[email protected]>
Co-authored-by: Alejandro Avilés <[email protected]>
Co-authored-by: Ergys Dona <[email protected]>
Co-authored-by: Vasant Vohra <[email protected]>
Co-authored-by: Javier Ferrer <[email protected]>
Co-authored-by: Giorgio Pieretti <[email protected]>
Co-authored-by: Javier Ferrer <[email protected]>
Co-authored-by: Parth Shandilya <[email protected]>
javfg added a commit to javfg/indico-checkin that referenced this pull request Jul 27, 2021
* Refactor code and add PKCE flow option
* Add and apply indico linting rules
* Use same date format everywhere
@javfg javfg mentioned this pull request Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve oauth security
2 participants